Skip to content

rustdoc: linking to a local proc macro no longer warns #141411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 28, 2025
Merged

rustdoc: linking to a local proc macro no longer warns #141411

merged 1 commit into from
May 28, 2025

Conversation

lolbinarycat
Copy link
Contributor

fixes #91274

tried to keep the fix general in case we ever have any other kind of item that occupies
multiple namespaces simultaniously.

@rustbot
Copy link
Collaborator

rustbot commented May 22, 2025

r? @GuillaumeGomez

rustbot has assigned @GuillaumeGomez.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels May 22, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lolbinarycat lolbinarycat force-pushed the rustdoc-link-proc-macro-91274 branch from f328ab0 to 415dfa7 Compare May 22, 2025 23:36
@GuillaumeGomez
Copy link
Member

Not sure it's a good idea to generalize it. We should only do it for items that can be in multiple namepaces at once (I can only think of macros for this case).

@lolbinarycat
Copy link
Contributor Author

@GuillaumeGomez why? the fact that an item is in multiple namespaces is already a pretty good indication that it is able to appear in multiple namespaces.

i'm not sure why i would want write code that asks "is this item, which appears in multiple namespaces, able to appear in multiple namespaces?"

technically tuple structs also appear in multiple namespaces (their constructor is a function), but i'm not sure if rustdoc sees that currently.

@GuillaumeGomez
Copy link
Member

It's mostly for performance reasons. We don't want to check items that are never part of multiple namespaces.

@lolbinarycat
Copy link
Contributor Author

Well, the code I added isn't run at all on the happy path, only in cases where the lint fires today, but I'll make it so the check is only enabled in proc-macro crates, since that just requires reading a field of TyCtx, and will eliminate the check in even more cases, since linking to a non-local proc macro already works fine (they're only functions in their defining crate).

@lolbinarycat
Copy link
Contributor Author

@GuillaumeGomez I thought of an edge case that my old solution wasn't handling, so I ended up adding a ui test and rewriting the whole thing.

Basically, instead of not emitting the error if there is proc-macro duplication, we instead deduplicate and then continue with the normal logic. this allows us to still provide clean and accurate error messages in the rare case where there is duplication, but there is also actual ambiguity (which is the situation the ui test creates).

@lolbinarycat lolbinarycat force-pushed the rustdoc-link-proc-macro-91274 branch from ed73524 to d688c80 Compare May 23, 2025 21:23
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lolbinarycat lolbinarycat force-pushed the rustdoc-link-proc-macro-91274 branch from d688c80 to cea63a6 Compare May 23, 2025 22:10
GuillaumeGomez
GuillaumeGomez reviewed May 24, 2025
View reviewed changes
GuillaumeGomez
GuillaumeGomez reviewed May 24, 2025
View reviewed changes
GuillaumeGomez
GuillaumeGomez reviewed May 24, 2025
View reviewed changes
@GuillaumeGomez
Copy link
Member

Apart from some nits, code looks better now. Making the change earlier is a very good idea. :)

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lolbinarycat lolbinarycat force-pushed the rustdoc-link-proc-macro-91274 branch from d2e2a64 to 7fc9166 Compare May 26, 2025 18:03
@GuillaumeGomez
Copy link
Member

Looks all good to me! Please squash your commits then I'll r+.

@lolbinarycat lolbinarycat force-pushed the rustdoc-link-proc-macro-91274 branch from 7fc9166 to 871327e Compare May 27, 2025 15:05
@lolbinarycat
Copy link
Contributor Author

@GuillaumeGomez squashed!

@GuillaumeGomez
Copy link
Member

Thanks!

Once CI passed, r=me.

@bors delegate=lolbinarycat

@bors
Copy link
Collaborator

bors commented May 27, 2025

✌️ @lolbinarycat, you can now approve this pull request!

If @GuillaumeGomez told you to "r=me" after making some further change, please make that change, then do @bors r=@GuillaumeGomez

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
##[group]Clock drift check
  local time: Tue May 27 16:05:45 UTC 2025
  network time: Tue, 27 May 2025 16:05:45 GMT
##[endgroup]
src/ci/docker/run.sh: line 384: 290196 Segmentation fault      (core dumped) docker cp checkout:/checkout/obj $objdir
##[error]Process completed with exit code 139.
Post job cleanup.

@lolbinarycat
Copy link
Contributor Author

@bors r=@GuillaumeGomez

@bors
Copy link
Collaborator

bors commented May 27, 2025

📌 Commit 871327e has been approved by GuillaumeGomez

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2025
tgross35 added a commit to tgross35/rust that referenced this pull request May 28, 2025
@tgross35
Rollup merge of rust-lang#141411 - lolbinarycat:rustdoc-link-proc-mac…
b1fe746 
…ro-91274, r=GuillaumeGomez

rustdoc: linking to a local proc macro no longer warns

fixes rust-lang#91274

tried to keep the fix general in case we ever have any other kind of item that occupies
multiple namespaces simultaniously.
@tgross35 tgross35 mentioned this pull request May 28, 2025
Closed
bors added a commit that referenced this pull request May 28, 2025
@bors
Auto merge of #141672 - tgross35:rollup-exs4f56, r=tgross35
a8edd4b 
Rollup of 8 pull requests

Successful merges:

 - #140369 (Add data_ptr method to Mutex and RwLock)
 - #140697 (Split `autodiff` into `autodiff_forward` and `autodiff_reverse`)
 - #141404 (Improve intrinsic handling in cg_ssa)
 - #141407 (Refactor the two-phase check for impls and impl items)
 - #141411 (rustdoc: linking to a local proc macro no longer warns)
 - #141548 (consider glob imports in cfg suggestion)
 - #141627 (Drop-build cleanups)
 - #141670 (Fix ICE in tokenstream with contracts from parser recovery)

r? `@ghost`
`@rustbot` modify labels: rollup
jhpratt added a commit to jhpratt/rust that referenced this pull request May 28, 2025
@jhpratt
Rollup merge of rust-lang#141411 - lolbinarycat:rustdoc-link-proc-mac…
c4fe0a5 
…ro-91274, r=GuillaumeGomez

rustdoc: linking to a local proc macro no longer warns

fixes rust-lang#91274

tried to keep the fix general in case we ever have any other kind of item that occupies
multiple namespaces simultaniously.
@jhpratt jhpratt mentioned this pull request May 28, 2025
Closed
bors added a commit that referenced this pull request May 28, 2025
@bors
Auto merge of #141672 - tgross35:rollup-exs4f56, r=tgross35
c067190 
Rollup of 8 pull requests

Successful merges:

 - #140369 (Add data_ptr method to Mutex and RwLock)
 - #140697 (Split `autodiff` into `autodiff_forward` and `autodiff_reverse`)
 - #141404 (Improve intrinsic handling in cg_ssa)
 - #141407 (Refactor the two-phase check for impls and impl items)
 - #141411 (rustdoc: linking to a local proc macro no longer warns)
 - #141548 (consider glob imports in cfg suggestion)
 - #141627 (Drop-build cleanups)
 - #141670 (Fix ICE in tokenstream with contracts from parser recovery)

r? `@ghost`
`@rustbot` modify labels: rollup
@tgross35 tgross35 mentioned this pull request May 28, 2025
Merged
bors added a commit that referenced this pull request May 28, 2025
@bors
Auto merge of #141696 - tgross35:rollup-fhpj180, r=tgross35
6f69710 
Rollup of 7 pull requests

Successful merges:

 - #140369 (Add data_ptr method to Mutex and RwLock)
 - #140697 (Split `autodiff` into `autodiff_forward` and `autodiff_reverse`)
 - #141404 (Improve intrinsic handling in cg_ssa)
 - #141411 (rustdoc: linking to a local proc macro no longer warns)
 - #141548 (consider glob imports in cfg suggestion)
 - #141627 (Drop-build cleanups)
 - #141670 (Fix ICE in tokenstream with contracts from parser recovery)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 77c7f76 into rust-lang:master May 28, 2025
12 of 13 checks passed
@rustbot rustbot added this to the 1.89.0 milestone May 28, 2025
rust-timer added a commit that referenced this pull request May 28, 2025
@rust-timer
Unrolled build for #141411
8a63b67 
Rollup merge of #141411 - lolbinarycat:rustdoc-link-proc-macro-91274, r=GuillaumeGomez

rustdoc: linking to a local proc macro no longer warns

fixes #91274

tried to keep the fix general in case we ever have any other kind of item that occupies
multiple namespaces simultaniously.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Milestone
1.89.0
Development

Successfully merging this pull request may close these issues.

rustdoc: broken_intra_doc_links is incorrectly reported when linking to a proc macro
5 participants
@lolbinarycat @rustbot @rust-log-analyzer @GuillaumeGomez @bors